[18.0][ADD] version control platform modules#3
Conversation
etobella
force-pushed
the
18.0-add-vcp
branch
2 times, most recently
from
e9c3515 to
18d1adc
Compare
|
Oh yeah! I am going to review and to contribute on it. Thanks ! |
etobella
force-pushed
the
18.0-add-vcp
branch
from
18d1adc to
a3bf522
Compare
vcp_github/models/res_partner.py
Outdated
| github_organization = fields.Boolean( | ||
| string="Is GitHub Organization", | ||
| help="Check if this partner represents a GitHub organization", | ||
| readonly=True, | ||
| ) | ||
| github_user = fields.Boolean( | ||
| string="Is GitHub User", | ||
| help="Check if this partner represents a GitHub user", | ||
| readonly=True, | ||
| ) |
There was a problem hiding this comment.
Proposal :
| github_organization = fields.Boolean( | |
| string="Is GitHub Organization", | |
| help="Check if this partner represents a GitHub organization", | |
| readonly=True, | |
| ) | |
| github_user = fields.Boolean( | |
| string="Is GitHub User", | |
| help="Check if this partner represents a GitHub user", | |
| readonly=True, | |
| ) | |
| github_user_type = fields.Selection( | |
| selection=[("user", "User"), ("organization", "Organization")], | |
| readonly=True, | |
| ) |
If I understand correctly, a github element can be an org or a user, not both.
don't you think ?
There was a problem hiding this comment.
Yes, but I did it this way because we use orgs as origin of the PR, and it can come from a user or from an org (some users are marked as organizations). However, I think your solution is cleaner (we might need to improve the view and so on)
legalsylvain
left a comment
legalsylvain
left a comment
There was a problem hiding this comment.
Some remarks inline.
Otherwise, LGTM. Thanks !
vcp/models/vcp_platform.py
Outdated
| def _cron_update_platforms(self): | ||
| for organization in self.search([]): | ||
| try: | ||
| organization.update_information() | ||
| except Exception as e: | ||
| _logger.error( | ||
| "Error updating organization %s: %s", organization.name, str(e) | ||
| ) |
There was a problem hiding this comment.
| def _cron_update_platforms(self): | |
| for organization in self.search([]): | |
| try: | |
| organization.update_information() | |
| except Exception as e: | |
| _logger.error( | |
| "Error updating organization %s: %s", organization.name, str(e) | |
| ) | |
| def _cron_update_platforms(self): | |
| for platform in self.search([]): | |
| try: | |
| platform.update_information() | |
| except Exception as e: | |
| _logger.error( | |
| "Error updating platform %s: %s", platform.name, str(e) | |
| ) |
vcp_management/models/res_partner.py
Outdated
|
|
||
| @api.depends() | ||
| def _compute_vcp_contributions(self): | ||
| self.filtered(lambda p: p.github_user)._compute_vcp_contributions_field( |
There was a problem hiding this comment.
hum. I think there is a little mistake in the design here. You call github_user field, but this one is defined in vcp_github module.
(same in line 33)
vcp_github/models/res_partner.py
Outdated
|
|
||
| _sql_constraints = [ | ||
| ( | ||
| "github_name_uniq", |
There was a problem hiding this comment.
I have a problem with that constraints, when I tested this module.
(The constraints, makes sense)
Use case :
- I have a database, with a partner named "sylvain LE GAL" (id#X)
- I install this module, and launch the fetch of github.
- As I can not set github_name on partner (github_name is readonly), the cron creates a new partner, named 'legalsylvain'. id#Y)
- When I try to merge the two partners. I have an error.
What do you think. This will be a common use case, IMO.
There was a problem hiding this comment.
@legalsylvain this comment will interest you #3 (comment)
There was a problem hiding this comment.
Yes. I had the same problem with the previous github_connector implementation. I implemented also a function to create partner on the fly. But there was some duplicates. I'm not sure what is the best solution.
sebastienbeau
left a comment
sebastienbeau
left a comment
There was a problem hiding this comment.
Thanks for you work, tell me what do you think about adding more granularity.
Note: on my side I was using Pygithub. I like doc and the way to get pull_request fell more python https://pygithub.readthedocs.io/en/stable/examples/PullRequest.html#get-pull-requests-by-query but It's a matter of taste, so I will follow you on whatever you choose
| _name = "vcp.platform" | ||
| _description = "VCP Platform" |
There was a problem hiding this comment.
Maybe a better name will be "vcp.orga" or "vcp.organization" ?
There was a problem hiding this comment.
well, actually, after some review I think we should do
vcp.platform.kind(Github, Gitlab, my own gitlab...)vcp.platform: The organization that we are analyzingvcp.organization: For handling companies
There was a problem hiding this comment.
I was asking myself the same question this night about vcp.platform and vcp.organization, in both case it's an organization but we do not have same usage. On github is the same table, but in Odoo we do not want to pollute the table of "vcp.platform" that we want to download all repo. I was thinking about having one table with a flag and a domain to hide "organization" in the menu, but it maybe a little breakable. So let's go to have "vcp.platform" and "vcp.organization"
vcp_github/models/res_partner.py
Outdated
| partner.github_organization = True | ||
| return partner.id | ||
|
|
||
| def _get_github_user(self, gh, client): |
There was a problem hiding this comment.
I am not sure that it's a good idea to create on the fly the partner.
At the initial import, some contributor may exist in the database but without having an existing github_login fill. So all PR will have the "duplicated partner" and we will need to merge it.
Also In the RFQ2 of the datamodel (see here: https://drive.google.com/file/d/1YCIce01J7P9unxAQ51XT7BqBIuZrOr9m/view) it's was required that a member can have several "github user". I am not sure that this case really exist in the reality, but it's was required.
On my side I was thinking about adding a simple model "vcp.user" (with some basic field and a link to a "partner_id") that will be created on the fly but then if it's linked to a "res.partner" we can compute the "partner_id" of the PR
The advantage I see:
- it's does create useless partner (In case of feature of partner-module-information, it do not want to have all oca contributor as partner in my ERP)
- it's allow to map the partner after
- it's allow to have several github-user
There was a problem hiding this comment.
We may use the same concept for the organization
There was a problem hiding this comment.
the new design is very better, and avoid creation of duplicates in res.partner table. thanks a lot !
| for pr in clients[i].search_issues( | ||
| f"is:pr repo:{self.platform_id.name}/{self.name} " | ||
| f"updated:{start.isoformat()}..{end.isoformat()}" | ||
| ): | ||
| i = (1 + i) % len(clients) |
There was a problem hiding this comment.
supporting multi key for solving rate limit ;) 👍
| def _parse_github_pr(self, pr, client): | ||
| origin_data = pr.as_dict() | ||
| comments_url = pr.comments_url | ||
| comments_req = client.session.get(comments_url) | ||
| comments = comments_req.json() | ||
| while comments_req.links.get("next"): | ||
| comments_url = comments_req.links["next"]["url"] | ||
| comments_req = client.session.get(comments_url) | ||
| comments += comments_req.json() | ||
| reviews_url = pr.reviews().url | ||
| reviews_req = client.session.get(reviews_url) | ||
| reviews = reviews_req.json() |
There was a problem hiding this comment.
As my aim is to reuse this module in other context
- gitlab interaction with project management
- module-partner-information : (for organizing the PR management + timesheet)
- odoo-repository to remove duplicated code
I was think about splitting more the module (so you can only depend on what you need)
To keep in mind
-
odoo-repository: only need the logic of vcp.plateform (or vcp.organisation), vcp.repository, vcp.branch
-
module-partner-information: same as odoo-repository + PR (without comment)
-
oca stats: need all
So my initial idea was to split a little more the module
- vcp
- vcp_pr
- vcp_pr_activity ( comment + review)
and so
- vcp_github
- vcp_github_pr
- vcp_github_pr_activity
To avoid a nightmare for the portal, I think we can include all the portal logic in vcp_pr_activity or creating a module vcp_portal that depend on vcp_pr_activity. But the idea is that the portal do not make sense without the activity information so we do need to split everything for the portal
What do you think ?
Note: I am not asking to do it (you already have done a lot), I can do it. I just want to get your point of view to go it the right direction
Note 2: I like the idea of having a portal for the member, and as we will have all the PR, I think we can improve it so we can show all the pending PR that you need to review, or all PR where somebody as you to fix it (I was talking about his with @legalsylvain some month ago)
There was a problem hiding this comment.
I will try to do the split, however, IMO, it has no sense to split pr from reviews and comments, they are quite aligned.
If as a company I want to monitor the PRs of OCA, it makes sense to know who approved them 😉
There was a problem hiding this comment.
Maybe @legalsylvain is right and we could do that by configuration (it might be simpler)
There was a problem hiding this comment.
There should be good to state clearly the goals /no goal of each project.
There is a high risk to build a gaz factory: hard to maintain, impossible to refactor.
what are odoo-repository and module-partner-information ? could you provide links ? |
Wahou that are a lot of split !
What do you think ? |
Here are the links : |
Having an option is the minimal needed (so we do not download all comment + review if you do not need it). I would like to have the feedback of @sebalix as the idea is to depend on this for the odoo-repository project (to avoid re-implementing the logic of orga, repo) I tend to prefer smaller module so it allow to share the migration effort. When you have a simple module as dependency you migrate it (less code, less work). If it does a lot of thing, you may wait for other to do it. But I will not block anything, I just share my point of view. |
|
The problem with your comment is that implies a lot of extra glue modules, we might need to find the best option that makes it feasible and not too complicated. For example, if we split too much, we might need vcp vcp_pr, vcp_review, vcp_github, vcp_github_pr, vcp_github_review.... that is the reason I commented, we need to find something that doesn't requires so many modules. |
Thinking twice, I tend to agree with you, splitting to much will add to much glue module, just having an option (on the vcp.plateform) can be enough for my case. And extracting the vcp_portal like you did already simplify the base module so it's not a big deal to migrate the comment and review. |
|
|
||
| class VcpBranch(models.Model): | ||
| _name = "vcp.branch" | ||
| _description = "Branch" |
There was a problem hiding this comment.
What is a branch exactly? Are we talking about a real git branch name, or an Odoo release/serie/version?
Asking as I faced this name issue in https://github.com/OCA/module-composition-analysis/blob/16.0/odoo_repository/models/odoo_branch.py which deserves a renaming to odoo.serie or odoo.version, but now the goal is to re-use your modules. In that project a serie/version is 17.0/18.0/19.0, while a branch can be main/18.0-mig which is linked to a serie/version.
@sebastienbeau you were proposing the term Odoo "serie" IIRC
There was a problem hiding this comment.
a Branch is a Branch in git. We should need to keep it, because there are repositories that are not using odoo versions (master/main for example on libraries like openupgradelib)
There was a problem hiding this comment.
@sebalix I think we should split the concept of "odoo.serie" and "vcp.branch".
An "odoo.serie" is the major version of odoo (17.0, 18.0, 19.0...)
A "vcp.branch" is a branch in term of git.
For sure in OCA case the branch (for repo that are odoo modules) are linked to one "odoo.version" that have the same name (but maybe one days if odoo start to be stable, we may have branch like 20+ that work with several version)
So my idea is that in odoo-repository we name this "odoo.version" and "odoo.serie" and then a branch is link to a serie (18.0-mig have the odoo.serie 18.0)
There was a problem hiding this comment.
All clear, wanted to know what it was about 👍
| origin_data = pr.as_dict() | ||
| comments_url = pr.comments_url | ||
| comments_req = client.session.get(comments_url) | ||
| comments = comments_req.json() | ||
| while comments_req.links.get("next"): | ||
| comments_url = comments_req.links["next"]["url"] | ||
| comments_req = client.session.get(comments_url) | ||
| comments += comments_req.json() |
There was a problem hiding this comment.
Just a minor comment (not blocking), but here we retrieve all comment, and review (even the old one that may have been already imported and processed on preview PR update).
This is not a big issue, we just have to keep in mind that at every PR update we will scan again all comment and review.
Note: it will just take more python time as the Odoo ORM will skip useless sql UPDATE (writing same data in a comment do not make any UPDATE)
There was a problem hiding this comment.
Yes, but it is also the only way to know if something changed on the comment
sebastienbeau
left a comment
sebastienbeau
left a comment
There was a problem hiding this comment.
I have tested the module and it work like expected.
We can still improve the view (for comment, review, maybe add menu). But it's not blocking for that we want to do.
@etobella thanks for your work !
etobella
force-pushed
the
18.0-add-vcp
branch
3 times, most recently
from
915dabe to
15e357c
Compare
|
@pedrobaeza @sebastienbeau I made the necessary changes to make it more compatible with interface_github. Obviously, there is still some work to do (like adding other fields and so on, but well, this is a Proof of Concept). Can you make a review? I tested adding 2 different platforms, odoo and OCA and everyhing went perfectly. |
etobella
force-pushed
the
18.0-add-vcp
branch
from
b9e3528 to
7135aed
Compare
|
@pedrobaeza I think that most of the functionality has been replicated. We should decide how to continue... 
|
|
I am going to review it this weekend. Thanks for your work and your time. |
There was a problem hiding this comment.
Hi. @etobella. Nice works !
- Some remarks inline.
- I made some trivial improvment here : dixmit#4 could you take a look and give me a feedback ?
I don't understand all the "rule / rule.information" design. Could you elaborate in witch case we want to analyse the same branch with different "process rules" ? thanks !(understood with your screenshot)
| type_id = fields.Many2one( | ||
| "vcp.host.type", | ||
| ) |
There was a problem hiding this comment.
| type_id = fields.Many2one( | |
| "vcp.host.type", | |
| ) | |
| host_type = fields.Selection() |
I don't understand the objective of the model vcp.host.type. I see a fields.Selection an implementation more simple. (no need to add a data at each time, only selection_add="github").
Did I missed something ?
| original_branches = self.branch_ids | ||
| existing_branches = {b.branch_id.name: b for b in self.branch_ids} | ||
| found_branches = self.env["vcp.repository.branch"] | ||
| for branch in repo.branches(): |
There was a problem hiding this comment.
Hi.
I don't see any way to filter branch to fetch. As a result, the algorythm will create a lot of noicy branches. See for exemple :
Don't you think ?
If agree, I can implement a branch pattern feature (as for process rules), but I'm not sure to understand the process rules implementations. And not sure at witch place implement that feature. (org and or repo).
There was a problem hiding this comment.
maybe we could add a regex, it might have sense 😉
There was a problem hiding this comment.
OK. I'll do it in dixmit#4 (I put in draft again).
|
@sebastienbeau said :
done in dixmit#4, with many other improvment, if you want to take a look ? thanks ! |
|
The commit history should be useful for later references, grouping things in logical batches. What is not useful is to see a lot of commits where you do and undo things while the development was on course and the PR hasn't been merged. I'm now checking the modules, so please wait a bit for merging. |
|
Hi @pedrobaeza. If we squash commits arbitrarily (by user, for example as @etobella proposed), we'll lose information, and we won't solve the issue you mentioned. (do and undo things, because @etobella have done some feature and I undid the features). So I don't understand what you propose. |
|
Yeah, in that case, the authorship prevails although it's not "so clean", but that doesn't invalidate the rest of the arguments for squashing the rest. |
|
@pedrobaeza I don't understand very well what you ask. Which commit should be merged and which not. Let's move on; this isn't the first time we've been unable to agree on your "squash request". @etobella : as you asked me : I don't see any interest of you squashing commits, for me and for the OCA contributors, and I don't want you to waste your time squashing commits. That giving said, as @sebastienbeau said " if you prefer to squash I am not opposed.". |
vcp_management/__manifest__.py
Outdated
|
|
||
| { | ||
| "name": "VCP Management", | ||
| "summary": """Management for your Virtual Control Platforms""", |
There was a problem hiding this comment.
| "summary": """Management for your Virtual Control Platforms""", | |
| "summary": "Management for your Version Control Platforms", |
There was a problem hiding this comment.
Thanks for the typo detection. there were some others 😢 fixed.
vcp_management/__manifest__.py
Outdated
| ], | ||
| "demo": [], | ||
| "external_dependencies": { | ||
| "python": ["GitPython", "pathspec"], |
There was a problem hiding this comment.
Why do you need here a library for Git? Didn't we talked about making this version control software agnostic? Example: having Mercurial repositories.
There was a problem hiding this comment.
That's true. I splitted the logic on a new module.
pedrobaeza
left a comment
pedrobaeza
left a comment
There was a problem hiding this comment.
I'm trying to put some cloc rules, and I don't get how to do it. Can you please put an example in demo and make some explanation in the readme?
vcp_github/readme/CONFIGURE.md
Outdated
| @@ -0,0 +1,10 @@ | |||
| [ This file is not always required; it should explain **how to configure the module before using it**; it is aimed at users with administration privileges. | |||
There was a problem hiding this comment.
These files should be completed. I'm missing an explanation about the configuration of the source path via system parameter vcp_management.source_code_local_path or the configuration or environment variable.
There was a problem hiding this comment.
Yes... this could be improved.
Done 😉
set kind field required at platform level. It doesn't makes sense to have a plateform without kind + it makes failing call of update_information AttributeError: 'vcp.platform' object has no attribute '_update_information_False'. Did you mean: '_update_information_github'?
Remove dependancy on website Create users and Organizations Remove portal dependancy Improve comments. Add Host Type Add groups Handle importing by cron add git url Add rules & cloc review Add dependancies Order of file loading Adding tests Copy with depth 1 Set default values, fix views
bad path for icon allow to delete a repository, if it has repository branch allow to fetch only 'sources' repositories, ignoring forks allow to fetch only 'active' repositories, ignoring archived repositories allow to delete a branch, if it has repository branch add last_commit_date (in github, it related to 'PushedAt' field. (https://github.com/orgs/community/discussions/24442). It's an important field that show if the repo is active, or totally abandonned Add new smart button to go from a platform to related repositories put vcp.platform.key model in dedicated file, applying OCA rule use new image.mixin, simplifying code base add branch count on repo level display all the versionsw where the module is available Add branch pattern, to fetch only branches whose names are matching a given pattern. It allows to ignore ocabot branches, in case of fetching OCA repositories use new menu items nested system Add vcp.branch menuitem, and improve display Add vcp.branch menuitem, and improve display. Fix body storage. (convert markdown into html) Add vcp.review menuitem, and improve display Add a menu entry for the vcp.request.label model. Add a Color field on labels. add link from vcp.request to vcp.review add link from vcp.request to vcp.comment get 'draft' state of pull requests. add a new 'status' field on vcp.request, that is based on 'is_draft', 'is_merged' and 'state' field to display the correct ribbon on form view / badge on list view allow to delete pull requests improve naming of field, renaming information_update into scheduled_information_update. Add a helper. Check the field by default, as it is the expected behaviour user want harmonize actions. put a path in all actions. rename xml_id of some actions. improve naming of field, renaming information_update into scheduled_information_update for vcp.repository model. Add helpers. explicit limit in cron python code. explicit limit in dron definition. add a filter to display only active requests. (draft and open) add vcp.repository.branch menu entry tests now include parameter limit
Improve texts improve readme. Add some fixes. split git logic
use users and partners Fix rename from vcp.platform.type to vcp.host Adapt to Host Type Fix tests add git url Adapt tests and code to get data
allow to fetch only 'sources' repositories, ignoring forks allow to fetch only 'active' repositories, ignoring archived repositories add last_commit_date (in github, it related to 'PushedAt' field. (https://github.com/orgs/community/discussions/24442). It's an important field that show if the repo is active, or totally abandonned adapt github mock to latest changes demo data Add branch pattern, to fetch only branches whose names are matching a given pattern. It allows to ignore ocabot branches, in case of fetching OCA repositories Add vcp.branch menuitem, and improve display. Fix body storage. (convert markdown into html) get 'draft' state of pull requests. add a new 'status' field on vcp.request, that is based on 'is_draft', 'is_merged' and 'state' field to display the correct ribbon on form view / badge on list view improve naming of field, renaming information_update into scheduled_information_update. Add an helper. Check the field by default, as it is the expected behaviour user want Improve naming of field, renaming information_update into scheduled_information_update for vcp.repository model. Add helpers. explicit limit in cron python code. do not create a branch when fetching pull request, if the target branch doesn't match the pattern
Improve error message Fix minor texts and readme Adapt to new VCP logic
Co-authored-by: Sylvain LE GAL <sylvain.legal@grap.coop>
use new image.mixin, simplifying code base delete a repository branch should unlink related odoo modules. Otherwise, if we recreate after a repository branch, it will create odoo modules duplicated Display number of versions in which the module is available. By default, set a filter to hide modules without versions display all the versionsw where the module is available create a python file per model, following OCA convention entry menu item for vcp.odoo.module.version set readonly on all 'vcp.odoo.xxx' model fields rename vcp.odoo.lib.python into vcp.odoo.python.library. (following similar logic with the modele bin.package) add two new menu entries for vcp.odoo.python.library and vcp.odoo.bin.package display on python.library and bin.package form views, the list of odoo.module.version that uses those libs (or bins) Allow to unlink modules fetch html description harmonize actions. put a path in all actions. rename xml_id of some actions.
etobella
changed the title
|
Ok! I made a split and merge of the commits by module and author to minimize the number of commits. for reference, we can get the history from the last commit: I think that it is almost ready. Let's wait for a last review from Pedro, let's go with it and after that I will review how to make a migration from interface-github to this to allow integration. It will be done on a different PR in order to avoid pollution of this one. |
8 participants
@sebastienbeau I think you could enjoy this one 😄